feat(QTDI-1305): improve error in record #1041
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yyin-talend
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
undx
reviewed
May 9, 2025
There was a problem hiding this comment.
As said in comments, I think that errors should be handled at record level not schema one.
The workflow expects that checking for bad values is done outside the framework.
Maybe, we could handle rather in it?
Another lead may be :
- define rules for bad data handling in schema
- use the usual
withTypein Record. (checking is done here as for null values handling).
wdyt?
component-api/src/main/java/org/talend/sdk/component/api/record/Record.java
Outdated
Show resolved
Hide resolved
| @@ -311,6 +313,11 @@ default Optional<Record> getOptionalRecord(final String name) { | |||
| return ofNullable(get(Record.class, name)); | |||
| } | |||
|
|
|||
| default boolean isValid() { | |||
| return !getSchema().getAllEntries() | |||
| .anyMatch(entry -> !entry.isValid()); | |||
There was a problem hiding this comment.
See sonar comment, more easy to read that double negation.
| /** | ||
| * @return true if the value of this entry is valid; false for invalid value. | ||
| */ | ||
| boolean isValid(); |
There was a problem hiding this comment.
Can also add a default behavior as returning true...
There was a problem hiding this comment.
It is not using schema, but using Entry.
Comment on lines
40
to
46
| String ENTRY_IS_ON_ERROR = "entry.on.error"; | ||
|
|
||
| String ENTRY_ERROR_MESSAGE = "entry.error.message"; | ||
|
|
||
| String ENTRY_ERROR_FALLBACK_VALUE = "entry.error.fallback.value"; | ||
|
|
||
| String ERROR_EXCEPTION = "entry.error.exception"; |
There was a problem hiding this comment.
see my previous comment. Maybe smtg like record.value.
| @@ -163,6 +163,10 @@ public String getProp(final String property) { | |||
| public Builder toBuilder() { | |||
| throw new UnsupportedOperationException("#toBuilder()"); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isValid() { return true; } | |||
| assertNotNull(entry2); | ||
| Assertions.assertFalse(entry2.isValid()); | ||
|
|
||
| System.setProperty(Record.RECORD_ERROR_SUPPORT, "false"); |
There was a problem hiding this comment.
You can use the before and after annotations to handle this...
|
|
||
| @Emitter | ||
| public SupportErrorInput createSource() { | ||
|
|
| @GridLayout(value = { | ||
| @GridLayout.Row("dataset"), | ||
| }) | ||
| @Version |
Comment on lines
237
to
254
| Schema customerSchema = schemaBuilder.withEntry(nameEntry).withEntry(nmEntry).withEntry(ageEntry).build(); | ||
| // record 1 | ||
| Record.Builder recordBuilder = factory.newRecordBuilder(customerSchema); | ||
| Record record1 = recordBuilder.withError("name", null, "Stirng is null", null) | ||
| .withString("normal", "normal") | ||
| .withError("age", "string", "is not an int", null) | ||
| .build(); | ||
| assertFalse(record1.isValid()); | ||
|
|
||
| final Schema.Entry entry = | ||
| record1.getSchema().getEntries().stream().filter(e -> "name".equals(e.getName())).findAny().get(); | ||
| assertNotNull(entry); | ||
| Assertions.assertFalse(entry.isValid()); | ||
|
|
||
| final Schema.Entry entry2 = | ||
| record1.getSchema().getEntries().stream().filter(e -> "age".equals(e.getName())).findAny().get(); | ||
| assertNotNull(entry2); | ||
| Assertions.assertFalse(entry2.isValid()); |
There was a problem hiding this comment.
Here, we're testing the Record/Schema implementations, not the Avro related.
Comment on lines
524
to
535
| // duplicate the schema instance with a modified Entry | ||
| final Entry oldEntry = this.findExistingEntry(columnName); | ||
| final Entry updateEntry = oldEntry.toBuilder() | ||
| .withName(columnName) | ||
| .withNullable(true) | ||
| .withType(oldEntry.getType()) | ||
| .withProp(SchemaProperty.ENTRY_IS_ON_ERROR, "true") | ||
| .withProp(SchemaProperty.ENTRY_ERROR_MESSAGE, errorMessage) | ||
| .withProp(SchemaProperty.ENTRY_ERROR_FALLBACK_VALUE, String.valueOf(value)) | ||
| .withProp(SchemaProperty.ERROR_EXCEPTION, exception == null ? "" : exception.toString()) | ||
| .build(); | ||
| return updateEntryByName(columnName, updateEntry); |
There was a problem hiding this comment.
This intent expects that value is always in error.
|
Discussed with Yves and forget my comments on Schema level vs Record Level. |
…lend/component-runtime into yyin/QTDI-1305-ImproveErrorInRecord-2
…lend/component-runtime into yyin/QTDI-1305-ImproveErrorInRecord-2
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…e record is valid.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
undx
previously approved these changes
May 16, 2025
undx
approved these changes
May 16, 2025
|
Analysis Details3 Issues
Coverage and Duplications
Project ID: org.talend.sdk.component:component-runtime |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Why this PR is needed?
What does this PR adds (design/code thoughts)?